Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OidcIdToken cannot be serialized to JSON if token contains claim of type JSONArray or JSONObject #9222

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 27, 2020

ObjectToListStringConverter and ObjectToMapStringObjectConverter were checking if the source object is of type List or Map and if the first element or key is a String. If we have a JSONArray containing Strings the above check will pass, meaning that a JSONArray will be returned which is not serializable (same applies to JSONObject)

With this change, even if the check is passing a new List or Map will be returned.

Closes gh-9210

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 27, 2020
@@ -54,7 +54,7 @@ public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor t
}
Map<?, ?> sourceMap = (Map<?, ?>) source;
if (!sourceMap.isEmpty() && sourceMap.keySet().iterator().next() instanceof String) {
Copy link
Author

@ghost ghost Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion this check is no longer required, as it will be covered by Map<String, Object> result = new HashMap<>(); sourceMap.forEach((k, v) -> result.put(k.toString(), v)); and a new Map will be created in both cases.

…ype JSONArray or JSONObject

ObjectToListStringConverter and ObjectToMapStringObjectConverter were checking if the source object is of type List or Map and if the first element or key is a String. If we have a JSONArray containing Strings the above check will pass, meaning that a JSONArray will be returned which is not serializable (same applies to JSONObject)

With this change, even if the check is passing a new List or Map will be returned.

Closes spring-projectsgh-9210
@jgrandja jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 3, 2020
@jgrandja jgrandja added this to the 5.5.0-M2 milestone Dec 3, 2020
@jgrandja
Copy link
Contributor

jgrandja commented Dec 3, 2020

Thanks for the PR @ovidiupopa91 ! This is now in master and backported to 5.2.x.

@jgrandja jgrandja closed this Dec 3, 2020
@jgrandja
Copy link
Contributor

jgrandja commented Dec 4, 2020

@ovidiupopa91 If you're available for another PR in Spring Security or Spring Authorization Server let me know and I'll find one for you.

@ghost
Copy link
Author

ghost commented Dec 4, 2020

Hi @jgrandja . Sure, I would be happy to contribute to a new PR. Either of them would be fine.

@jgrandja
Copy link
Contributor

jgrandja commented Dec 4, 2020

Thanks @ovidiupopa91 ! I will be in touch next week with a couple of options to choose from . Enjoy your weekend!

@jgrandja
Copy link
Contributor

jgrandja commented Dec 8, 2020

@ovidiupopa91 Here are a couple of options to choose from:

OAuth 2.0 Mutual-TLS Client Authentication
Client spring-security#4498
Server spring-authorization-server#101

OpenID Connect Dynamic Client Registration 1.0
spring-authorization-server#57

Do any of these features interest you?

Keep in mind that these are major features and will require some effort and time.
I'm not sure what your free time is like but keep this in mind when deciding.

If you prefer a smaller feature let me know and I'll find something.

FYI, I'll be on PTO starting Dec 11 and returning Jan 4.

@ghost
Copy link
Author

ghost commented Dec 9, 2020

Hi @jgrandja. I can give it a try with

OpenID Connect Dynamic Client Registration 1.0
spring-authorization-server#57

When it comes to my free time, I'm usually allocating 2-3hours/day. I don't have a problem regarding the effort, but it also depends when this feature is planned to be rolled out. If this must happen in the next 1-2 weeks I'm not sure if I can finish till then.

Let me know if this is ok with you too. Maybe we can have a chat before you leave, discussing this feature before actually starting to implement it. (we can do it on gitter)

I will also be on PTO starting Dec 21 and returning on Jan 6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OidcIdToken cannot be serialized to JSON if token contains claim of type JSONArray
4 participants